Open
Conversation
- Add _setStoredBlobHash helper and BATCH_BLOB_VERSIONED_HASHES_SLOT in L1MessageBaseTest - Preset batchBlobVersionedHashes in tests so commitBatch succeeds without blob tx - test_commitAndFinalizeWithL1Messages_succeeds: set stored blob hash for batch 1 and 2 - test_commitBatches_succeeds: set stored blob hash for batch 1 - test_revertBatch_succeeds: set stored blob hash for batch 1 and 2 - Remove duplicate ZERO_VERSIONED_HASH from RollupCommitBatchWithProofTest Made-with: Cursor
Co-authored-by: FletcherMan <fanciture@163.com> Co-authored-by: fletcher.fan <fletcher.fan@bitget.com>
Contributor
📝 WalkthroughWalkthroughThis PR introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Submitter as Rollup Service
participant RollupContract as Rollup Contract
participant L1Chain as L1 Chain
participant PendingPool as Pending TX Pool
Note over Submitter: Decide Commit Method
Submitter->>RollupContract: Check batchBlobVersionedHashes[nextBatchIndex]
RollupContract-->>Submitter: stored hash exists?
alt Stored Hash Exists (after revert)
Submitter->>Submitter: Select commitState method
Submitter->>Submitter: Create TX without blob data
else No Stored Hash (fresh commit)
Submitter->>L1Chain: Read blobhash(0)
L1Chain-->>Submitter: blob hash
Submitter->>Submitter: Select commitBatch or commitBatchWithProof
Submitter->>Submitter: Include blob hash in TX
end
Submitter->>RollupContract: Execute commit TX
RollupContract->>RollupContract: Store blobVersionedHash for batch
RollupContract-->>Submitter: TX confirmed
Submitter->>PendingPool: Track pending TX
Note over PendingPool: Monitor for confirmation
PendingPool-->>Submitter: TX confirmed / failed
alt TX Succeeds
Submitter->>Submitter: Mark confirmed, update state
else TX Already Committed Error
Submitter->>RollupContract: Rebuild: reuse stored hash
Submitter->>Submitter: Retry with commitState
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tx-submitter/services/rollup.go (2)
386-405:⚠️ Potential issue | 🔴 CriticalNil/uninitialized rotator handling is still inconsistent.
These lines now treat
r.rotator == nilor missingstartTime/epochas a safe skip, butrollup()still unconditionally callsr.rotator.CurrentSubmitter(...)and dereferencesr.rotator.startTime/epoch. With the same configuration the background rollup loop will panic instead of skipping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tx-submitter/services/rollup.go` around lines 386 - 405, The rollup() logic calls r.rotator.CurrentSubmitter and later dereferences r.rotator.startTime/epoch even when r.rotator is nil or uninitialized, risking a panic; update rollup() to first check r.cfg.PriorityRollup and r.rotator != nil before calling r.rotator.CurrentSubmitter, and also guard access to r.rotator.startTime and r.rotator.epoch (returning errNotMyTurn when rotator is nil or its state fields are nil) so all uses of r.rotator are protected; specifically adjust the conditional around r.cfg.PriorityRollup and r.rotator, move the CurrentSubmitter call after that guard, and ensure any early returns use consistent error values.
602-688:⚠️ Potential issue | 🟠 MajorReverted batches can leave doomed
commitBatchtxs pending until timeout.This doomed-commit check only looks at
batchIndex <= lastCommitted. AfterrevertBatch,lastCommitteddrops back below the batch again, butRollup.commitBatchnow reverts whilebatchBlobVersionedHashes[batchIndex]is set. A recovered/pendingcommitBatchtherefore sits untilTxTimeoutbeforeReSubmitTxcan rebuild it tocommitState, which can stall recommit or burn a nonce on-chain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tx-submitter/services/rollup.go` around lines 602 - 688, handlePendingTx can leave doomed commitBatch txs pending when a revertBatch leaves lastCommitted < batchIndex but batchBlobVersionedHashes[batchIndex] remains set; update the commit-like branch in handlePendingTx to also detect this reverted-but-has-blob state and cancel immediately: after computing batchIndex and lastCommitted (using utils.ParseParentBatchIndex and r.Rollup.LastCommittedBatchIndex), add a check against the rollup's batch blob map (batchBlobVersionedHashes or an existing accessor) and treat a present/non-empty blob entry for batchIndex as a reason to cancel (same CancelTx flow used when batchIndex <= lastCommitted); reference handlePendingTx, CancelTx, commitBatch, revertBatch, and batchBlobVersionedHashes when making the change.tx-submitter/services/pendingtx.go (1)
242-299:⚠️ Potential issue | 🟠 MajorRecover
pnoncefrom the max nonce, not the last journal entry.Line 294 assumes
ParseAllTxs()returns nonce order, but journal append order can contain replacements with a lower nonce after a higher one. After restart that leavespnoncetoo low and the next submission can reuse an already-pending nonce.💡 Suggested fix
- var maxCommitBatchIndex, maxFinalizeBatchIndex uint64 + var maxCommitBatchIndex, maxFinalizeBatchIndex, maxNonce uint64 for _, tx := range txs { + if tx.Nonce() > maxNonce { + maxNonce = tx.Nonce() + } + // Get method name method := utils.ParseMethod(tx, abi) @@ - pt.SetNonce(txs[len(txs)-1].Nonce()) + pt.SetNonce(maxNonce)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tx-submitter/services/pendingtx.go` around lines 242 - 299, The recovery currently sets pnonce from the last journal entry via pt.SetNonce(txs[len(txs)-1].Nonce()), which is unsafe; instead compute the maximum nonce seen while iterating txs and call pt.SetNonce(maxNonce) after the loop. Update the loop that processes txs to track a maxNonce (compare using tx.Nonce()), handle the empty-txs case appropriately, and replace the pt.SetNonce(...) call with pt.SetNonce(maxNonce) so pnonce reflects the highest pending nonce rather than the last array element.
🧹 Nitpick comments (2)
contracts/contracts/test/base/L1MessageBase.t.sol (1)
50-54: Storage slot constants are fragile — consider using foundry's storage inspection.These hardcoded slot numbers (172, 173) will silently break if the Rollup contract's storage layout changes. Consider adding a comment noting how these were derived (e.g.,
forge inspect Rollup storage-layout) or adding a sanity-check assertion insetUp()that verifies the slots are still correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/contracts/test/base/L1MessageBase.t.sol` around lines 50 - 54, The hardcoded storage slot constants BATCH_BLOB_VERSIONED_HASHES_SLOT and ROLLUP_DELAY_PERIOD_SLOT (and ZERO_VERSIONED_HASH) are fragile; run and record the exact command used to derive them (e.g., `forge inspect Rollup storage-layout`) in a comment next to the constants, and add a sanity-check assertion in setUp() that uses vm.load against the deployed Rollup contract to verify those slots contain the expected sentinel values (compare the loaded bytes32 to ZERO_VERSIONED_HASH or known defaults) so tests fail fast if the Rollup storage layout changes.tx-submitter/mock/rollup.go (1)
68-71: Mock always returns empty hash —commitStatepath untestable.The mock always returns
[32]byte{}, which meansuseCommitState(checked viastoredBlobHash != [32]byte{}inrollup.go:1163) will always befalse. This prevents testing thecommitStatecode path.Consider adding a setter method to control the return value:
Proposed addition
type MockRollup struct { lastCommittedBatchIndex *big.Int lastFinalizedBatchIndex *big.Int insideChallengeWindow bool batchExists bool batchTx *types.Transaction finalizeTx *types.Transaction + blobVersionedHash [32]byte } // BatchBlobVersionedHashes implements IRollup (no stored hash by default) func (m *MockRollup) BatchBlobVersionedHashes(opts *bind.CallOpts, batchIndex *big.Int) ([32]byte, error) { - return [32]byte{}, nil + return m.blobVersionedHash, nil } +// SetBlobVersionedHash sets the mock value for BatchBlobVersionedHashes +func (m *MockRollup) SetBlobVersionedHash(hash [32]byte) { + m.blobVersionedHash = hash +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tx-submitter/mock/rollup.go` around lines 68 - 71, The mock BatchBlobVersionedHashes currently always returns an empty [32]byte, which prevents the rollup code path that checks storedBlobHash != [32]byte{} (useCommitState) from being exercised; add a field to MockRollup (e.g., batchBlobVersionedHashesRet [32]byte) and a setter method (e.g., SetBatchBlobVersionedHashesHash(hash [32]byte)) that sets that field, then update BatchBlobVersionedHashes to return the stored field value instead of always returning [32]byte{} so tests can toggle useCommitState by setting a non-zero hash.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/contracts/test/Rollup.t.sol`:
- Around line 1396-1406: The test test_commitState_reverts_when_carrying_blob
currently sends a non-blob batch and only asserts success, so it doesn't
exercise the "blobhash(0) != 0" revert; change the test to explicitly assert the
revert path by constructing a BatchDataInput where blobhash(0) is non-zero
(e.g., bytes32(uint256(1))) and wrap the commit call with
hevm.expectRevert("commitState must not carry blob") (or vm.expectRevert if your
test harness uses vm), then hevm.prank(alice); call
rollup.commitState(batchDataInput, batchSignatureInput) to ensure the revert is
triggered; optionally keep the existing non-blob assert as a separate test or
split into two cases so both success and revert paths are covered (referencing
test_commitState_reverts_when_carrying_blob, batchDataInput,
rollup.commitState).
In `@tx-submitter/services/pendingtx.go`:
- Around line 279-299: The recovery rewrite is not crash-safe because pt.dump()
calls journal.Clean() before re-appending entries from pt.txinfos, risking
truncation if a crash or append fails; fix by making dump() atomic: write the
deduplicated entries to a temporary journal (or append to a new file) and fsync
the temp, then swap/rename it over the live journal (or only call
journal.Clean() after successful writes), ensuring pt.dump() (and any calls to
journal.Clean()/journal.Append()) only truncates the original after the new data
is durably persisted; update references in pendingtx.go to use the
atomic-temp-and-rename approach inside pt.dump() so pt.txinfos remains the
authoritative source until the swap completes.
In `@tx-submitter/services/reorg.go`:
- Around line 31-37: The inline comment and the value for maxHistory in
NewReorgDetector disagree (comment says "Track last 50 blocks" but maxHistory is
5); fix by making them consistent: either set maxHistory to 50 to match the
comment in NewReorgDetector or change the comment to reflect tracking 5 blocks.
Update the unique symbol NewReorgDetector and its maxHistory field so the value
and comment match.
---
Outside diff comments:
In `@tx-submitter/services/pendingtx.go`:
- Around line 242-299: The recovery currently sets pnonce from the last journal
entry via pt.SetNonce(txs[len(txs)-1].Nonce()), which is unsafe; instead compute
the maximum nonce seen while iterating txs and call pt.SetNonce(maxNonce) after
the loop. Update the loop that processes txs to track a maxNonce (compare using
tx.Nonce()), handle the empty-txs case appropriately, and replace the
pt.SetNonce(...) call with pt.SetNonce(maxNonce) so pnonce reflects the highest
pending nonce rather than the last array element.
In `@tx-submitter/services/rollup.go`:
- Around line 386-405: The rollup() logic calls r.rotator.CurrentSubmitter and
later dereferences r.rotator.startTime/epoch even when r.rotator is nil or
uninitialized, risking a panic; update rollup() to first check
r.cfg.PriorityRollup and r.rotator != nil before calling
r.rotator.CurrentSubmitter, and also guard access to r.rotator.startTime and
r.rotator.epoch (returning errNotMyTurn when rotator is nil or its state fields
are nil) so all uses of r.rotator are protected; specifically adjust the
conditional around r.cfg.PriorityRollup and r.rotator, move the CurrentSubmitter
call after that guard, and ensure any early returns use consistent error values.
- Around line 602-688: handlePendingTx can leave doomed commitBatch txs pending
when a revertBatch leaves lastCommitted < batchIndex but
batchBlobVersionedHashes[batchIndex] remains set; update the commit-like branch
in handlePendingTx to also detect this reverted-but-has-blob state and cancel
immediately: after computing batchIndex and lastCommitted (using
utils.ParseParentBatchIndex and r.Rollup.LastCommittedBatchIndex), add a check
against the rollup's batch blob map (batchBlobVersionedHashes or an existing
accessor) and treat a present/non-empty blob entry for batchIndex as a reason to
cancel (same CancelTx flow used when batchIndex <= lastCommitted); reference
handlePendingTx, CancelTx, commitBatch, revertBatch, and
batchBlobVersionedHashes when making the change.
---
Nitpick comments:
In `@contracts/contracts/test/base/L1MessageBase.t.sol`:
- Around line 50-54: The hardcoded storage slot constants
BATCH_BLOB_VERSIONED_HASHES_SLOT and ROLLUP_DELAY_PERIOD_SLOT (and
ZERO_VERSIONED_HASH) are fragile; run and record the exact command used to
derive them (e.g., `forge inspect Rollup storage-layout`) in a comment next to
the constants, and add a sanity-check assertion in setUp() that uses vm.load
against the deployed Rollup contract to verify those slots contain the expected
sentinel values (compare the loaded bytes32 to ZERO_VERSIONED_HASH or known
defaults) so tests fail fast if the Rollup storage layout changes.
In `@tx-submitter/mock/rollup.go`:
- Around line 68-71: The mock BatchBlobVersionedHashes currently always returns
an empty [32]byte, which prevents the rollup code path that checks
storedBlobHash != [32]byte{} (useCommitState) from being exercised; add a field
to MockRollup (e.g., batchBlobVersionedHashesRet [32]byte) and a setter method
(e.g., SetBatchBlobVersionedHashesHash(hash [32]byte)) that sets that field,
then update BatchBlobVersionedHashes to return the stored field value instead of
always returning [32]byte{} so tests can toggle useCommitState by setting a
non-zero hash.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c9cf0df-78cd-4d89-bd4f-b5ec573f6fb2
📒 Files selected for processing (19)
bindings/bin/rollup_deployed.hexbindings/bin/zkevmverifierv1_deployed.hexbindings/bindings/rollup.gobindings/bindings/rollup_more.gobindings/bindings/zkevmverifierv1.gobindings/bindings/zkevmverifierv1_more.gocontracts/contracts/l1/rollup/IRollup.solcontracts/contracts/l1/rollup/Rollup.solcontracts/contracts/test/Rollup.t.solcontracts/contracts/test/base/L1MessageBase.t.soltx-submitter/batch/batch_restart_test.gotx-submitter/constants/methods.gotx-submitter/iface/rollup.gotx-submitter/mock/rollup.gotx-submitter/services/pendingtx.gotx-submitter/services/reorg.gotx-submitter/services/rollup.gotx-submitter/services/rollup_handle_test.gotx-submitter/utils/utils.go
Comment on lines
+1396
to
+1406
| /// @dev commitState must be rejected when tx carries blob (blobhash(0) != 0). | ||
| /// Contract: require(blobhash(0) == bytes32(0), "commitState must not carry blob"). | ||
| /// In test env we cannot send a blob tx (blobhash(0) is always 0), so we only assert that without blob commitState succeeds. | ||
| /// Full revert test requires a blob transaction. | ||
| function test_commitState_reverts_when_carrying_blob() public { | ||
| _setupCommitStatePrecondition(); | ||
| batchDataInput = IRollup.BatchDataInput(0, _genesisBatchHeader(), 1, 0, stateRoot, stateRoot, bytes32(uint256(4))); | ||
| hevm.prank(alice); | ||
| rollup.commitState(batchDataInput, batchSignatureInput); | ||
| // Without blob in tx, commitState succeeds. Revert "commitState must not carry blob" is hit only when blobhash(0) != 0 (blob tx). | ||
| assertEq(rollup.lastCommittedBatchIndex(), 1); |
Contributor
There was a problem hiding this comment.
This test does not exercise the blob rejection path.
The function name says “reverts when carrying blob”, but the body sends a non-blob tx and asserts success. If the blobhash(0) guard is removed, this test still passes, so it gives false coverage.
💡 Minimal cleanup
- function test_commitState_reverts_when_carrying_blob() public {
+ function test_commitState_succeeds_without_blob() public {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/contracts/test/Rollup.t.sol` around lines 1396 - 1406, The test
test_commitState_reverts_when_carrying_blob currently sends a non-blob batch and
only asserts success, so it doesn't exercise the "blobhash(0) != 0" revert;
change the test to explicitly assert the revert path by constructing a
BatchDataInput where blobhash(0) is non-zero (e.g., bytes32(uint256(1))) and
wrap the commit call with hevm.expectRevert("commitState must not carry blob")
(or vm.expectRevert if your test harness uses vm), then hevm.prank(alice); call
rollup.commitState(batchDataInput, batchSignatureInput) to ensure the revert is
triggered; optionally keep the existing non-blob assert as a separate test or
split into two cases so both success and revert paths are covered (referencing
test_commitState_reverts_when_carrying_blob, batchDataInput,
rollup.commitState).
Comment on lines
+279
to
+299
| // Add to in-memory map only; do not write to journal yet. | ||
| // The original journal data is preserved until dump() succeeds below, | ||
| // so a crash here is safe — the next restart will re-read the original entries. | ||
| pt.mu.Lock() | ||
| pt.txinfos[tx.Hash()] = &types.TxRecord{ | ||
| Tx: tx, | ||
| SendTime: uint64(time.Now().Unix()), | ||
| QueryTimes: 0, | ||
| Confirmed: false, | ||
| } | ||
| pt.mu.Unlock() | ||
| } | ||
|
|
||
| pt.SetPindex(maxCommitBatchIndex) | ||
| pt.SetPFinalize(maxFinalizeBatchIndex) | ||
| pt.SetNonce(txs[len(txs)-1].Nonce()) | ||
|
|
||
| // Rewrite the journal with the deduplicated in-memory set. | ||
| // This replaces any duplicate entries accumulated by previous buggy restarts. | ||
| if err := pt.dump(); err != nil { | ||
| return fmt.Errorf("failed to rewrite journal after recovery: %w", err) |
Contributor
There was a problem hiding this comment.
The new recovery rewrite is not crash-safe.
Lines 279-281 say the original journal is preserved until dump() succeeds, but dump() starts with journal.Clean() and only then re-appends entries. A crash or append error in between can truncate the journal and lose still-pending txs on the next restart.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tx-submitter/services/pendingtx.go` around lines 279 - 299, The recovery
rewrite is not crash-safe because pt.dump() calls journal.Clean() before
re-appending entries from pt.txinfos, risking truncation if a crash or append
fails; fix by making dump() atomic: write the deduplicated entries to a
temporary journal (or append to a new file) and fsync the temp, then swap/rename
it over the live journal (or only call journal.Clean() after successful writes),
ensuring pt.dump() (and any calls to journal.Clean()/journal.Append()) only
truncates the original after the new data is durably persisted; update
references in pendingtx.go to use the atomic-temp-and-rename approach inside
pt.dump() so pt.txinfos remains the authoritative source until the swap
completes.
Comment on lines
+31
to
37
| func NewReorgDetector(l1Client iface.Client) *ReorgDetector { | ||
| return &ReorgDetector{ | ||
| blockHistory: make([]blockInfo, 0), | ||
| maxHistory: 5, // Track last 50 blocks | ||
| l1Client: l1Client, | ||
| metrics: metrics, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Comment/code mismatch: maxHistory is 5, not 50.
The inline comment on line 34 states "Track last 50 blocks" but maxHistory is set to 5. Please correct either the comment or the value to match the intended behavior.
Proposed fix
func NewReorgDetector(l1Client iface.Client) *ReorgDetector {
return &ReorgDetector{
blockHistory: make([]blockInfo, 0),
- maxHistory: 5, // Track last 50 blocks
+ maxHistory: 5, // Track last 5 blocks
l1Client: l1Client,
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func NewReorgDetector(l1Client iface.Client) *ReorgDetector { | |
| return &ReorgDetector{ | |
| blockHistory: make([]blockInfo, 0), | |
| maxHistory: 5, // Track last 50 blocks | |
| l1Client: l1Client, | |
| metrics: metrics, | |
| } | |
| } | |
| func NewReorgDetector(l1Client iface.Client) *ReorgDetector { | |
| return &ReorgDetector{ | |
| blockHistory: make([]blockInfo, 0), | |
| maxHistory: 5, // Track last 5 blocks | |
| l1Client: l1Client, | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tx-submitter/services/reorg.go` around lines 31 - 37, The inline comment and
the value for maxHistory in NewReorgDetector disagree (comment says "Track last
50 blocks" but maxHistory is 5); fix by making them consistent: either set
maxHistory to 50 to match the comment in NewReorgDetector or change the comment
to reflect tracking 5 blocks. Update the unique symbol NewReorgDetector and its
maxHistory field so the value and comment match.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
commitState()function enabling batch state commitment using stored blob hashesVERSION()function to verifier contractBug Fixes
Chores